Make some usages of suggestName runtime deprecated by mwachs5 · Pull Request #2556 · chipsalliance/chisel

mwachs5 · 2022-06-02T16:24:20Z

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

code cleanup

API Impact

This runtime deprecates some usages of the suggestName API and warns that they will be turned into errors in the future. Specifically, you can no longer:

call suggestName on the same HasId more than once
call suggestName after containing Module close
call suggestName with the same value that the prefix (autoSeed) would have given it
call suggestName outside of a builder context
Call suggestName on something that is not actually nameable (such as a subfield of an aggregate)

Backend Code Generation Impact

No impact on output verilog, though people avoiding these runtimeDeprecations would see them.

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Some usages of suggestName are runtime deprecated or made an error

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

mwachs5 · 2022-06-02T16:24:45Z

Opening this as draft since I am still working on the tests, which are revealing to me that there are myriad usages of suggestName inside the chisel internals itself that I was not aware of

mwachs5 · 2022-06-03T09:59:35Z

Note this currently comments out reflective naming as it kept tripping up the "you are suggesting the same name the plugin would have given you" test

core/src/main/scala/chisel3/internal/Builder.scala

mwachs5 · 2022-06-19T23:14:07Z

This has changed in that I now am checking that thing-you-are-suggestNaming is actually canBeNamed (brought this back from #2561 ), and no longer checking "has name already been used before suggestname", because i'm not sure the suggestion for that given that we don't want to waste space on _computedName... probably simpler to just check if we have already closed the module

* suggestName -- add some runtime deprecations * suggestName -- make it an error to call suggestName outside of Builder Context * SuggestName: create an internal version and use it internally to avoid runtime deprecations * Added a SuggestNameSpec to test suggestName behavior

…cause they are hitting the 'you get the same name as the prefix' warnings

…chiselName

src/main/scala/chisel3/util/Arbiter.scala

src/main/scala/chisel3/util/Decoupled.scala

core/src/main/scala/chisel3/Module.scala

This reverts commit cd07ede.

core/src/main/scala/chisel3/internal/Builder.scala

jackkoenig

Some minor suggestions, and 1 problem we should sort out (the error message for the suggestName matching the automatically derived name).

core/src/main/scala/chisel3/internal/Builder.scala

src/main/scala/chisel3/util/Decoupled.scala

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/naming/SuggestNameSpec.scala

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/naming/SuggestNameSpec.scala

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/naming/SuggestNameSpec.scala

core/src/main/scala/chisel3/internal/Builder.scala

Co-authored-by: Jack Koenig <koenig@sifive.com>

…tandalone PR

Co-authored-by: Jack Koenig <koenig@sifive.com>

src/test/scala/chiselTests/naming/SuggestNameSpec.scala

…t-name-errors

core/src/main/scala/chisel3/internal/Builder.scala

src/test/scala/chiselTests/naming/SuggestNameSpec.scala

core/src/main/scala/chisel3/internal/Builder.scala

mwachs5 · 2022-07-02T03:15:27Z

I think I've incorporated all the feedback

core/src/main/scala/chisel3/internal/Builder.scala

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

mwachs5 added this to the 3.5.x milestone Jun 2, 2022

mwachs5 force-pushed the suggest-name-errors branch 2 times, most recently from 936b66e to 9b6e3fe Compare June 3, 2022 09:57

mwachs5 marked this pull request as ready for review June 3, 2022 09:59

mwachs5 changed the title ~~[WIP] Make some usages of suggestName runtime deprecated~~ Make some usages of suggestName runtime deprecated Jun 3, 2022

jackkoenig mentioned this pull request Jun 3, 2022

Add --warn:reflective-naming #2561

Merged

16 tasks

jackkoenig reviewed Jun 3, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jun 3, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Show resolved Hide resolved

mwachs5 force-pushed the suggest-name-errors branch from 062bb28 to 1cd71a2 Compare June 27, 2022 20:46

mwachs5 added 10 commits June 28, 2022 12:52

no more _computedName

08543f3

post merge cleanups

236a4fd

Deleted a bunch of chiselName and TransitName to get tests to pass be…

eaedd38

…cause they are hitting the 'you get the same name as the prefix' warnings

Add back the check for suggestName after module close

87f0e41

Add another case that we can name IOs

f0d2499

scalafmt

a175c1d

scalafmt

aceaae4

Add back the @chiselName

3918123

SuggestNameSpec: use something other than Queue to avoid issues with …

dbb9078

…chiselName

mwachs5 force-pushed the suggest-name-errors branch from 1cd71a2 to dbb9078 Compare June 28, 2022 20:00

mwachs5 commented Jun 28, 2022

View reviewed changes

src/main/scala/chisel3/util/Arbiter.scala Show resolved Hide resolved

mwachs5 commented Jun 28, 2022

View reviewed changes

src/main/scala/chisel3/util/Decoupled.scala Show resolved Hide resolved

mwachs5 commented Jun 28, 2022

View reviewed changes

core/src/main/scala/chisel3/Module.scala Show resolved Hide resolved

mwachs5 added 2 commits June 28, 2022 13:38

I don't need to suggestName on clock/reset do I?

cd07ede

Revert "I don't need to suggestName on clock/reset do I?"

a0d145d

This reverts commit cd07ede.

jackkoenig reviewed Jun 29, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved

jackkoenig requested changes Jun 30, 2022

View reviewed changes

mwachs5 and others added 7 commits June 30, 2022 13:55

Update core/src/main/scala/chisel3/internal/Builder.scala

a8b296f

Co-authored-by: Jack Koenig <koenig@sifive.com>

Arbiter restore chiselName

412a8d1

restore nowarn annotation in Decoupled, that should be deleted in a s…

bbc0d0a

…tandalone PR

Update core/src/main/scala/chisel3/internal/Builder.scala

d7a1782

Co-authored-by: Jack Koenig <koenig@sifive.com>

Update core/src/main/scala/chisel3/internal/Builder.scala

898ae25

Co-authored-by: Jack Koenig <koenig@sifive.com>

Update core/src/main/scala/chisel3/internal/Builder.scala

5f25d90

Co-authored-by: Jack Koenig <koenig@sifive.com>

Update core/src/main/scala/chisel3/internal/Builder.scala

5a11980

Co-authored-by: Jack Koenig <koenig@sifive.com>

mwachs5 commented Jul 1, 2022

View reviewed changes

src/test/scala/chiselTests/naming/SuggestNameSpec.scala Outdated Show resolved Hide resolved

mwachs5 added 4 commits July 1, 2022 11:20

Update src/test/scala/chiselTests/naming/SuggestNameSpec.scala

0075c25

Merge remote-tracking branch 'origin/suggest-name-errors' into sugges…

7b84a38

…t-name-errors

checkpoint: one test fails

db9152f

scalafmt

7cf0c42

mwachs5 commented Jul 1, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved

mwachs5 commented Jul 1, 2022

View reviewed changes

src/test/scala/chiselTests/naming/SuggestNameSpec.scala Show resolved Hide resolved

mwachs5 added 2 commits July 1, 2022 14:26

fix numbering

ce18389

scalafmt

7158448

jackkoenig reviewed Jul 1, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved

mwachs5 added 4 commits July 1, 2022 16:51

get the builder context check to work

e60b9bd

scalafmt

9772b29

avoid any allocations in seedOpt check

67ce7d8

Merge branch 'master' into suggest-name-errors

fa28909

mwachs5 commented Jul 2, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/Builder.scala Outdated Show resolved Hide resolved

Update core/src/main/scala/chisel3/internal/Builder.scala

47d88ce

jackkoenig pushed a commit that referenced this pull request Feb 28, 2023

Update sbt-mima-plugin to 1.1.1 (#2556)

9c85b3c

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Conversation

mwachs5 commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Uh oh!

mwachs5 commented Jun 2, 2022

Uh oh!

mwachs5 commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwachs5 commented Jun 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mwachs5 commented Jul 2, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwachs5 commented Jun 2, 2022 •

edited

Loading

mwachs5 commented Jun 3, 2022 •

edited

Loading